Pages List performance improvements #22070

crazytonyli · 2023-11-22T01:50:32Z

Fixes #21813. Also, in general, this PR makes it's possible to load a site with a large number of pages (300+).

Issue

Currently Pages List performs very poorly on a site with a large number of pages. A pretty extreme case is #21813. The app may become unresponsive in 5 minutes or so, when about 6000 pages are loaded.

If you have a test site with 300 pages, you can open that site in the app and go to Pages screen and the app will become unresponsive very quickly.

If you don't have a test site with that many pages, you can use FG as the test site, with a little bit of code change: adding another "and" condition to this if statement: && loadedPosts.count < 500. The number 500 is the max of pages you want to the app to fetch. You can tweak this number to test the Pages List's performance against certain number of pages.

Changes

There are a few issues involved:

Pages are loaded in memory and populated into UI after all pages are fetched from API. This gives a false sense of Pages List stuck at "Fetching pages...".
Pages are saved using the main context. Meaning all the Core Data operations, inserting, updating, deleting, happen on the main thread.
Pages are displayed in hierarchical order. But the current algorithm doesn't perform well. See Add a new PageTree which will be used to display pages list #22056.

The first two issues are addressed by the PostRepository.fetchAllPages function. And the last issues is addressed by the linked PR #22056. This PR basically uses those new implementations to swap out the current problematic ones.

That means, this PR should not bring any UI changes to the app, it only addresses performance issues.

Note

It'd be much easier to review this PR commit by commit.

Further considerations

In other discussions, the general consensus is it's bad to load all pages automatically, instead of page by page as user scrolls through the list (like Posts List). @kean mentioned we could change Pages List to match WP.com, where the pages are displayed in a hierarchical order if there are certain number in total (say 600), but they are displayed page by page (like Posts List) if the total amount exceed this threshold.

I agree with that idea, but did not implement it in this PR, because I don't want this PR to get too large. I'm not sure if I'd have time to implement it though, because I have other project lined up (dropping Alamofire from WordPressKit).

Test Instructions

Verify #21813 is fixed. Use FG to test Pages List performance.

This PR also adds a cancellation mechanism to the Pages List, where the Pages List stops loading more pages when user taps the "Back" button. You can verify this behaviour by using Xcode's "Network Activity Report" in the Debug panel: You should see periodic spikes in the Network Activity when the app are on FG's pages list and see no more spikes when exits the pages list screen.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
Test using FG.
What automated tests I added (or what prevented me from doing so)
None.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist: N/A

wpmobilebot · 2023-11-22T02:22:24Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr22070-73baed8
	Version	23.7
	Bundle ID	org.wordpress.alpha
	Commit	`73baed8`
	App Center Build	WPiOS - One-Offs #7956

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-11-22T02:27:17Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr22070-73baed8
	Version	23.7
	Bundle ID	com.jetpack.alpha
	Commit	`73baed8`
	App Center Build	jetpack-installable-builds #6980

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

mokagio

Leaving some notes before moving on with trying this on device.

mokagio · 2023-11-24T04:24:16Z

WordPress/Classes/Services/PostRepository.swift

-    func fetchAllPages(statuses: [BasePost.Status], in blogID: TaggedManagedObjectID<Blog>) -> Task<[TaggedManagedObjectID<Page>], Swift.Error> {
+    func fetchAllPages(statuses: [BasePost.Status], authorUserID: NSNumber? = nil, in blogID: TaggedManagedObjectID<Blog>) -> Task<[TaggedManagedObjectID<Page>], Swift.Error> {


Nitpick: The Swift API Guidelines recommend to locate parameters with default values at the end of the list:

Prefer to locate parameters with defaults toward the end of the parameter list. Parameters without defaults are usually more essential to the semantics of a method, and provide a stable initial pattern of use where methods are invoked.

Good catch. I used default parameter value quite freely in a few similar functions in this class. I want to keep the blog parameter as the last parameter (or the first), and rest of the parameters (which act as post/pages filters) don't really have an sensible order. But these functions are added one by one across many PRs, it's probably worth looking into these function holistically later.

Sounds good

mokagio · 2023-11-24T04:25:42Z

WordPress/Classes/Utility/PageTree.swift

+        struct PageData {
+            var postID: NSNumber?
+            var parentID: NSNumber?
+        }


Always like a container type for information like this one.

Do the properties need to be mutable?

Maybe no? I used var out of habit without any extra thoughts. However, it's used as a constant though: let pageData: PageData.

mokagio · 2023-11-24T04:30:50Z

WordPress/Classes/ViewRelated/Post/AbstractPostListViewController.swift

@@ -475,85 +475,80 @@ class AbstractPostListViewController: UIViewController,
        return .any
    }

-    func syncHelper(_ syncHelper: WPContentSyncHelper, syncContentWithUserInteraction userInteraction: Bool, success: ((_ hasMore: Bool) -> ())?, failure: ((_ error: NSError) -> ())?) {
+    @MainActor
+    func syncPosts(isFirstPage: Bool) async throws -> ([AbstractPost], Bool) {


From the function signature, I couldn't understand what that Bool in the returned tuple represents. I only figured it out by looking at the usage, where it's assigned to hasMore.

Have you considered defining a struct to model that information?

Addressed in 5981dcf. I used a named tuple instead of a struct.

mokagio · 2023-11-24T04:31:55Z

WordPress/Classes/ViewRelated/Pages/PageListViewController.swift

+        refreshResults()
+    }
+
+    /// Build page hierachy in background, which should not take long (less than 2 seconds for 6000+ pages).


Show off 😄

😅 Taking 2 seconds to sort 6000 items is not really fast though...It's much faster than what we have at the moment, but still not fast enough. I'm being lazy here by putting them into a background thread. The comment here is mainly pointing out, the sorting isn't fast enough to take place in the main thread, but is fast enough to put in the background thread without worrying it keep going forever.

mokagio · 2023-11-24T04:35:49Z

WordPress/Classes/ViewRelated/Post/AbstractPostListViewController.swift

+                guard let (posts, hasMore) = try await self?.syncPosts(isFirstPage: false) else { return }
+
+                guard let self else { return }


Would it make sense to swap these? The end result would be the same, return if self is not available, but I feel it's odd to access self? and check let self afterwards. 🤷‍♂️

Suggested change

guard let (posts, hasMore) = try await self?.syncPosts(isFirstPage: false) else { return }

guard let self else { return }

guard let self else { return }

let (posts, hasMore) = try await self.syncPosts(isFirstPage: false)

I didn't try this code, maybe I'm missing something and in fact it's necessary to do guard twice.

This guard let self is similar to how we do the same check in a completion block. In reality, self should never be nil when calling syncPosts, but could be nil if user has exit the screen before the sycnPosts returns. I have added a comment in 19307cc

mokagio · 2023-11-24T04:38:51Z

WordPress/WordPressTest/PagesListTests.swift

-            let pageTree = PageTree()
-            pageTree.add(pages)
-            let list = pageTree.hierarchyList()
+            let list = (try? PageTree.hierarchyList(of: pages)) ?? []


I was going to suggest the following, which I feel would make the point at which the test might have failed.

Suggested change

let list = (try? PageTree.hierarchyList(of: pages)) ?? []

let list = try XCTUnwrap(try? PageTree.hierarchyList(of: pages)))

But I checked the docs and I guess it's not possible to throw from the block given to measure 😞

Yep, that's why I added an assertion below, to make sure we are measuring success cases, not failure cases.

mokagio

Worked great on my iPhone 15 Pro.

I took videos for comparison, then remembered the site to test this edge case is a private one 🥷

Anyways, on the same device, the 23.7 beta load two pages in the list then nothing else happens in the UI. In this build, the rest of the pages appear after a second or so.

Great work 👏

peril-wordpress-mobile · 2023-11-27T00:39:14Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

wpmobilebot · 2023-11-27T00:47:08Z

	1 Warning
⚠️	This PR is assigned to the milestone 23.8. This milestone is due in less than 4 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

mokagio · 2023-11-27T00:55:37Z

Enabled auto-merge so this can land in trunk ASAP and we can move forward with the 23.8 code freeze.

crazytonyli added Performance Pages List labels Nov 22, 2023

crazytonyli added this to the 23.8 milestone Nov 22, 2023

crazytonyli requested a review from mokagio November 22, 2023 01:50

crazytonyli self-assigned this Nov 22, 2023

crazytonyli changed the title ~~Tonyli use repository pages list~~ Pages List performance improvements Nov 22, 2023

crazytonyli added 4 commits November 22, 2023 14:53

Use PostRepository to fetch posts in Posts List and Pages List

d5c98e6

Decouple PageTree from specific managed object context

e2f4172

Use PageTree in Pages List

eef9c21

Add a release note

1a32c7a

crazytonyli force-pushed the tonyli-use-repository-pages-list branch from c90bf98 to 1a32c7a Compare November 22, 2023 01:53

Merge branch 'trunk' into tonyli-use-repository-pages-list

09b060a

mokagio reviewed Nov 24, 2023

View reviewed changes

mokagio approved these changes Nov 24, 2023

View reviewed changes

crazytonyli added 2 commits November 27, 2023 13:38

Put sync posts result into a named tuple

5981dcf

Add a code comment

19307cc

Merge branch 'trunk' into tonyli-use-repository-pages-list

73baed8

mokagio enabled auto-merge November 27, 2023 00:55

mokagio merged commit 6a1517a into trunk Nov 27, 2023
23 checks passed

mokagio deleted the tonyli-use-repository-pages-list branch November 27, 2023 01:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pages List performance improvements #22070

Pages List performance improvements #22070

crazytonyli commented Nov 22, 2023

wpmobilebot commented Nov 22, 2023 •

edited

Loading

wpmobilebot commented Nov 22, 2023 •

edited

Loading

mokagio left a comment

mokagio Nov 24, 2023

crazytonyli Nov 27, 2023

mokagio Nov 27, 2023

mokagio Nov 24, 2023

crazytonyli Nov 27, 2023 •

edited

Loading

mokagio Nov 24, 2023

crazytonyli Nov 27, 2023

mokagio Nov 24, 2023

crazytonyli Nov 27, 2023

mokagio Nov 24, 2023

crazytonyli Nov 27, 2023

mokagio Nov 24, 2023

crazytonyli Nov 27, 2023

mokagio left a comment

peril-wordpress-mobile bot commented Nov 27, 2023

wpmobilebot commented Nov 27, 2023

mokagio commented Nov 27, 2023

		func fetchAllPages(statuses: [BasePost.Status], in blogID: TaggedManagedObjectID<Blog>) -> Task<[TaggedManagedObjectID<Page>], Swift.Error> {
		func fetchAllPages(statuses: [BasePost.Status], authorUserID: NSNumber? = nil, in blogID: TaggedManagedObjectID<Blog>) -> Task<[TaggedManagedObjectID<Page>], Swift.Error> {

		guard let (posts, hasMore) = try await self?.syncPosts(isFirstPage: false) else { return }

		guard let self else { return }

	let list = (try? PageTree.hierarchyList(of: pages)) ?? []
	let list = try XCTUnwrap(try? PageTree.hierarchyList(of: pages)))

Pages List performance improvements #22070

Pages List performance improvements #22070

Conversation

crazytonyli commented Nov 22, 2023

Issue

Changes

Further considerations

Test Instructions

Regression Notes

wpmobilebot commented Nov 22, 2023 • edited Loading

wpmobilebot commented Nov 22, 2023 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazytonyli Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio left a comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Nov 27, 2023

wpmobilebot commented Nov 27, 2023

mokagio commented Nov 27, 2023

wpmobilebot commented Nov 22, 2023 •

edited

Loading

wpmobilebot commented Nov 22, 2023 •

edited

Loading

crazytonyli Nov 27, 2023 •

edited

Loading